[DONT MERGE] externally implementable items #146348
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-attributes
This comment has been minimized.
This comment has been minimized.
jdonszelmann
commented
Sep 8, 2025
| // `SymbolExportLevel::Rust` export level but may end up being exported in dylibs. | ||
| || codegen_attrs.flags.contains(CodegenFnAttrFlags::USED_COMPILER) | ||
| || codegen_attrs.flags.contains(CodegenFnAttrFlags::USED_LINKER) | ||
| // Right now, the only way to get "foreign item symbol aliases" is by being an EII-implementation. |
There was a problem hiding this comment.
TODO: do we need this:
- option 1: we do and implement our own reachability analysis based on it separate from
RUSTC_STD_INTERNAL_SYMBOL. option 2: we don't cause we also useRUSTC_STD_INTERNAL_SYMBOLon all EIIs. In that case we should renameSTD_INTERNAL_SYMBOL- option 3: same as option 2 but maybe we shouldn't use
RUSTC_STD_INTERNAL_SYMBOLin the first place; we leave it as-is and create a new flag thta's like it but specifically for EIIs and named something else likeRUSTC_USED_BY_NONDIRECT_DEP_CRATEor whatever nicer name we can think of that (as long as it doesn't use STD since it's not really specific to that anymore)
There was a problem hiding this comment.
despite STD_INTERNAL_SYMBOL working, and we can test with it for a bit, this is not the way we should do it. i.e. not option 2.
jdonszelmann
commented
Sep 8, 2025
|
|
||
| #[eii(eii1)] | ||
| pub fn decl1(x: u64) { | ||
| //~^ WARN function `decl1` is never used |
There was a problem hiding this comment.
It took me a little bit to decide what is the expected behavior here. We might want to remove this warning. Defaults are in some way expected to be unused. However, here we can prove it because the explicit impl is in the same crate hence the warning.
There was a problem hiding this comment.
removing the warning actually makes the code a tiny bit trickier
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors2 try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust-bors bot
added a commit
that referenced
this pull request
Sep 9, 2025
[DONT MERGE] externally implementable items
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (d5a6633): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.7%, secondary 3.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 3.1%, secondary -1.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 468.052s -> 471.14s (0.66%) |
rustbot
added
perf-regression
|
Well, shit. I think I know some fixes but I hoped this wouldn't happen |
jdonszelmann
commented
Sep 10, 2025
|
☔ The latest upstream changes (presumably #146666) made this pull request unmergeable. Please resolve the merge conflicts. |
lcnr
reviewed
Sep 17, 2025
lcnr
reviewed
Sep 17, 2025
lcnr
reviewed
Sep 17, 2025
lcnr
reviewed
Sep 17, 2025
lcnr
reviewed
Sep 17, 2025
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #146221) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
EII collection queries
|
The job Click to see the possible cause of the failure (guessed by this bot)For more information how to resolve CI failures of this job, visit this link. |
oli-obk
requested changes
Oct 25, 2025
There was a problem hiding this comment.
Please leave some comments on the tests stating what they are testing. I can usually deduce it from the source, but then I'm left scratching my head when I see two very similar ones.
I'm just as much at fault of writing tests like this, but I also know I have ended up scratching my head while looking at them half a year later
| @@ -0,0 +1,16 @@ | |||
| error[E0050]: `other` has 0 parameters but #[foo] requires it to have 1 | |||
There was a problem hiding this comment.
We already have signature comparison logic for trait vs impl mismatches. Can we reuse that as it's already been quite tuned to show lots of details and make suggestions
There was a problem hiding this comment.
so the logic I used is for a large part the same, you'll see many signatures match and I call into it a few times. The reason it's different, is that we have to check there are no generics, we don't deal with a self type, and diagnostics can't refer to a trait obviously. I tried reusing parts, but I'm not sure we can much more than I do right now.
| /// This function is an implementation of an externally implementable item (EII). | ||
| /// This means, there was an EII declared somewhere and this function is the | ||
| /// implementation that should be run when the declaration is called. | ||
| pub eii_impls: ThinVec<EiiImpl>, |
There was a problem hiding this comment.
Why is it a vec and not an option?
There was a problem hiding this comment.
one function could be the implementation of multiple EIIs. A panic handler could also be the integer overflow handler simultaneously
There was a problem hiding this comment.
Hmm... Doesn't sound too compelling to support, but if the complexity is basically just in the collector/checker (where it's just a loop, so not a problem) and not in the macros or such, then it's ok.
Def needs tests tho. Also a test using the same eii attribute twice. Or one function defining two different eiis at the same time
| span, | ||
| vis: ast::Visibility { span, kind: ast::VisibilityKind::Inherited, tokens: None }, | ||
| kind: ast::ItemKind::ForeignMod(ast::ForeignMod { | ||
| extern_span: span, |
There was a problem hiding this comment.
Could be computed from the fn's extern span, everything but the generated Rust has one.
Various other uses of the fallback span may also have better spans to be used
There was a problem hiding this comment.
Not too excited about some of the parser stuff, but I think that's just the status of most builtin macros rn
| ribs: &[Rib<'ra>], | ||
| ignore_binding: Option<NameBinding<'ra>>, | ||
| ) -> Option<LexicalScopeBinding<'ra>> { | ||
| assert!(ns == TypeNS || ns == ValueNS); |
There was a problem hiding this comment.
Are there tests for various situations with naming conflicts?
There was a problem hiding this comment.
hm, unsure, In theory there shouldn't be any issue, EII attributes are simply part of the macro namespace and act exactly as other macros. The reason this assertion had to be removed is that we use the defid of that macro as an identifier for the EII itself - with is we look up its signature when type checking for example. And we used to effectively assert that all defids of macros were filtered out at this point.
compiler/rustc_metadata/src/eii.rs
Outdated
|
|
||
| // iterate over all items in the current crate | ||
| // FIXME(speed up) | ||
| for id in tcx.hir_crate_items(()).definitions() { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
compiler/rustc_metadata/src/eii.rs
Outdated
|
|
||
| // iterate over all items in the current crate | ||
| // FIXME(speed up) | ||
| for id in tcx.hir_crate_items(()).definitions() { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supersedes #140010
Tracking issue: #125418
Getting started: